Conversation
internal/informer/manager.go
Outdated
| // Start starts the informer factory and waits for all caches to sync. | ||
| // This method blocks until caches are synchronized or the context is canceled. | ||
| func (m *Manager) Start(ctx context.Context) error { | ||
| m.mu.Lock() |
There was a problem hiding this comment.
locking seems to be excessive here as it should be started once on startup and stopped later. Consider just removing it
| m.mu.Lock() | ||
| defer m.mu.Unlock() | ||
|
|
||
| if m.started { |
There was a problem hiding this comment.
if the informers have been already started, m.cancelFunc will not be nil, so it's enough just to check for that. Also based on anticipated usage, it seems also excessive
There was a problem hiding this comment.
would like to disagree - i prefer verbosity over the context
internal/informer/manager.go
Outdated
| }() | ||
|
|
||
| m.log.Info("starting shared informer factory...") | ||
| m.factory.Start(stopCh) |
There was a problem hiding this comment.
ctx.Done() can be passed instead of dedicated stopCh. And that go routine can be removed as in Stop() m.factory.Shutdown() can be used to wait for the informers to stop when the ctx is canceled
internal/informer/manager.go
Outdated
| m.log.Info("waiting for informer caches to sync...") | ||
| if !cache.WaitForCacheSync(syncCtx.Done(), m.nodes.HasSynced, m.pods.HasSynced) { | ||
| cancel() | ||
| metrics.IncrementInformerCacheSyncs("node", "failure") |
There was a problem hiding this comment.
I think these metrics are excessive because if the start fails on startup, then the whole process fails, right?
internal/informer/manager.go
Outdated
| } | ||
|
|
||
| // IsStarted returns true if the informer manager has been started and caches are synced. | ||
| func (m *Manager) IsStarted() bool { |
There was a problem hiding this comment.
do we have a real use case for this?
internal/informer/manager_test.go
Outdated
| clientset := fake.NewClientset(node, pod) | ||
| manager := NewManager(log, clientset, 0) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) |
There was a problem hiding this comment.
consider using t.Context() instead of context.Background()
internal/informer/manager_test.go
Outdated
| clientset := fake.NewClientset() | ||
| manager := NewManager(log, clientset, 0) | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
it seems table tests could be used for this case instead of having two separate tests which mostly redundant logic
| "k8s.io/client-go/kubernetes/fake" | ||
| ) | ||
|
|
||
| func TestNodeInformer_Informer(t *testing.T) { |
There was a problem hiding this comment.
looks like these all methods could be checked in a single test?
3 participants
No description provided.